Conversation
…e, and grep filtering
…te related functionality
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the log subcommand to directly read log files from the CLI, bypassing the daemon socket. It introduces new features such as following logs in real-time (-f), specifying the number of lines to display (-n), and viewing daemon or all service logs. Feedback on these changes highlights a few critical issues: a race condition in the follow loop that can cause duplicate log output, the unintended omission of empty lines in the output, incorrect handling of -n 0 line counts, and a lack of validation for invalid -n arguments.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the logging system in ziginit to allow the CLI to directly read log files instead of querying the daemon via Unix sockets. This change enables a new follow mode (-f), custom line counts (-n), and direct viewing of daemon logs. The feedback highlights a potential silent data loss bug in followOutput for log lines exceeding 4KB, dangerous silent path truncations in path-building functions that should panic instead, and an inefficiency in followFiles where files are repeatedly opened and closed in the polling loop.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the log command in ziginit to directly read log files from the CLI, bypassing the daemon socket. It introduces new features such as follow mode (-f), custom line counts (-n), and unified logging (--all), along with comprehensive E2E tests. The review feedback highlights a potential data loss bug in the carry buffer handling of journal.zig when incoming data exceeds available space, an unsafe @ptrCast for sentinel-terminated pointers, and a compatibility issue in the Go tests due to the use of the min built-in introduced in Go 1.21.
kooksee
left a comment
kooksee
left a comment
There was a problem hiding this comment.
审查摘要
文件覆盖
| 文件 | 已审查 | 问题数 |
|---|---|---|
| .github/instructions/ziginit.instructions.md | ✅ | 0 |
| .github/workflows/release-ziginit.yml | ✅ | 0 |
| tools/ziginit/README.md | ✅ | 0 |
| tools/ziginit/config.zig | ✅ | 0 |
| tools/ziginit/docs/log-improvements.md | ✅ | 0 |
| tools/ziginit/helpers.zig | ✅ | 0 |
| tools/ziginit/journal.zig | ✅ | 1 |
| tools/ziginit/logrotate.zig | ✅ | 0 |
| tools/ziginit/main.zig | ✅ | 0 |
| tools/ziginit/posix.zig | ✅ | 0 |
| tools/ziginit/protocol.zig | ✅ | 1 |
| tools/ziginit/server.zig | ✅ | 0 |
| tools/ziginit/service.zig | ✅ | 0 |
| tools/ziginit/tests/e2e_test.go | ✅ | 0 |
| tools/ziginit/tests/helpers_test.go | ✅ | 0 |
总计: 已审查 15/15 个文件
问题统计
| 严重程度 | 数量 |
|---|---|
| 🔴 关键 | 1 |
| 🟠 高 | 0 |
| 🟡 中 | 0 |
| 🟢 普通 | 1 |
| 🔵 低 | 0 |
关键风险
tools/ziginit/journal.zig的followOutput在 carry 非空且新 chunk 无换行时，若data.len > avail会静默丢弃 remainder，导致 follow 输出丢日志。
非阻断建议处置
| 建议 | 分类 | 处置 | 说明 |
|---|---|---|---|
| 协议注释与实现保持一致 | [CONS] | accepted | 已记录，建议修正文档注释为 2-byte msg_len。 |
Reviewer 对齐摘要
| 外部评论 | 分类 | 处置 | 理由 |
|---|---|---|---|
| 3330317269/7270/7271/7273 | [LOGI]/[RBST] | accepted | 对应修复已体现在当前代码。 |
| 3330455951/5954/5955 | [RBST]/[PERF] | accepted | 路径溢出防护与 follow FD 优化已落地。 |
| 3330472251 | [LOGI] | deferred | 当前实现仍可在 data.len > avail 时丢弃数据，需继续修复。 |
| 3330472254 | [PRAC] | rejected | 风格建议，现实现不构成功能缺陷。 |
| 3330472256 | [CPT] | rejected | 仓库 Go 版本 1.25.3，min 可用。 |
结论
存在 1 个阻断性逻辑问题（日志跟随模式可能静默丢数据），建议修复后再通过。
（本次使用降级通道提交：gh pr review --comment 等价的 PR review comment，因当前环境无 codereview MCP 工具）
1 participant
No description provided.